-
-
Notifications
You must be signed in to change notification settings - Fork 243
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Adds .vue functionality #77
Conversation
First of all: good work! What I say should be taken with a pinch of salt as I'm not a maintainer of this plugin. That aside, I notice that with the current approach the code moves to always bring Vue into the mix. I'd be tempted to refactor the code to only use the Vue approach when you are definitely using the Vue functionality. Essentially treating the Vue functionality as something that can be feature flagged on or off. Do you understand what I mean? (I may not be being super clear) |
@johnnyreilly Yeah I was thinking about that too. Do you think a flag to turn it on via the plugin options would be the way to go? |
I think that might be an idea (unless it can be reliably inferred somehow - possibly from the presence of |
src/IncrementalChecker.ts
Outdated
* Assumes wildcard "@" is [project root]/src. | ||
*/ | ||
static resolveNonTsModuleName(moduleName: string, containingFile: string, programConfigFile: string): string { | ||
if (moduleName.indexOf('@/') === 0) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of hardcoded values, both webpack configuration (resolve.alias, resolve.modules) and tsconfig parameters (baseUrl and paths option) could be exploited here.
src/IncrementalChecker.ts
Outdated
@@ -9,6 +9,7 @@ import WorkSet = require('./WorkSet'); | |||
import NormalizedMessage = require('./NormalizedMessage'); | |||
import CancellationToken = require('./CancellationToken'); | |||
import minimatch = require('minimatch'); | |||
import vueParser = require('vue-parser'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better to use vue-template-compiler parseComponent function. Return type is SFCDescriptor, see flow typings https://github.com/vuejs/vue/blob/dev/flow/compiler.js#L172-L177
I don't think we can rely on |
Fair point - maybe an explicit flag is better |
Maybe we can check vue-loader is available by trying to simply trying to require it ... |
@Toilal That's a pretty clever idea (smart detection). How do you guys feel about me moving all of this code into a separate module called There's no reason to be super DRY here. We can let Vue be its own thing and go it's own path by taking care of it in it's own module. Leave the normal program as it was in IncrementalChecker before I touched it. Then I'll start applying all those suggestions you made throughout the code @Toilal. Thoughts? |
Sounds promising! |
Alright guys, Everything works great on my example project on my local machine because the project is a Vue project with Update: Nevermind! facepalm Thanks @Toilal |
src/IncrementalChecker.ts
Outdated
const projectPath = path.dirname(this.programConfigFile); | ||
const contextResolution = Resolve.sync('vue-loader', { basedir: projectPath }); | ||
|
||
return require(contextResolution) ? true : false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems you still have to try/catch this line and return false on error ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh right! Brain freeze! Of course that's the problem. LOL. Will fix...
voila! @Toilal ! All checks passing now. I'll merge master into it (and fix any conflicts) after all feedback is in. Now regarding feedback, I looked over webpack resolve and so far I don't really see any API documentation around that for using it programmatically. Also, for |
Alright guys, there's a huge line of eager people waiting for this (there's over 1,300 people who have TSLint Vue currently installed in their VSCode editor and those Vue folks are almost all guaranteed to be on Webpack). And there's no other option availabe for Vue developers that runs at the typechecker level. Since I've split off the code so it doesn't disrupt the original program, what can we do to fast-track this PR? I'll continue to help polish things up afterwards as well. |
@prograhammer the person who needs to feed back is @piotr-oles. This is his project and only he can merge I'm afraid. In the short term you could consider publishing the fork to a GitHub repo with the compiled Speaking of which, I must take that down now 0.2.9 has shipped. |
Background to how that came to pass here: #64 |
Ah, gotcha. No worries! Yeah I've got this branch here that I'll keep promoting to folks in the meantime: |
Sweet! |
src/IncrementalChecker.ts
Outdated
} | ||
|
||
nextIteration() { | ||
if (!this.watcher) { | ||
this.watcher = new FilesWatcher(this.watchPaths, ['.ts', '.tsx']); | ||
this.watcher = new FilesWatcher(this.watchPaths, ['.ts', '.tsx', '.vue']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could the .vue
be conditional based on whether vue is in the mix or not please?
src/IncrementalChecker.ts
Outdated
nextIteration() { | ||
if (!this.watcher) { | ||
this.watcher = new FilesWatcher(this.watchPaths, ['.ts', '.tsx']); | ||
this.watcher = new FilesWatcher(this.watchPaths, ['.ts', '.tsx', '.vue']); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could including vue
be conditional upon whether this is a vue
program or not please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it hurt to always include .vue
? It doesn't really bother the folks not on Vue.js. Otherwise it's another condition we'll need to make here for the watcher. Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 things really:
- It will make a marginal speed difference for non-Vue users. I'm philosophically opposed to that 😄
- It bothers me on an OCD level that Vue files would be always in the mix when they needn't be. This is more subjective I grant you but makes me twitch in unhappy ways. It is my suffering.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's always a battle between code readability and micro-optimization (in this case the optimization may be significant). OCD kicks in for both 😄
But let me see of a clean way to condition that...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks man!
src/IncrementalChecker.ts
Outdated
@@ -158,7 +176,14 @@ class IncrementalChecker { | |||
} | |||
} | |||
|
|||
this.program = IncrementalChecker.createProgram(this.programConfig, this.files, this.watcher, this.program); | |||
if (this.hasVueLoader()) { | |||
this.programConfig = this.programConfig || VueProgram.loadProgramConfig(this.programConfigFile); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in this branch isn't it always the VueProgram
you use?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no, this condition is here to detect if vue-loader
is found in the user's project and then use the VueProgram if it is. We don't need to maintain a separate branch for Vue if we have this condition. It can be merged into master.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.programConfig || VueProgram.loadProgramConfig(this.programConfigFile)
but wouldn't this statement always use programConfig
? Or have I missed something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first pass over that line this.programConfig
will be undefined so it will grab the return from the right side of the condition (VueProgram.loadProgramConfig(...
). The next time it passes over that line this.programConfig
will have a value and it will grab that instead of performing another unnecessary VueProgram.loadProgramConfig(...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool - WFM!
It's looking good @prograhammer - I've a few comments as you can see but it's basically good. I'm glad that this is mostly triggered only when Vue is in the mix. @piotr-oles would you be able to take a look at this when you get a mo? |
Thanks for the effort @prograhammer When importing a ERROR in /app/TSModule.vue(2,27):
TS2306: File '/app/JSModule.vue' is not a module. Same result regardless of Importing vue components with TS works fine from both plain EDIT: declare module '*.vue' {
import Vue from 'vue'
export default Vue
} |
Please accept that! |
@JBBr A couple things to point out here:
|
@johnnyreilly All your feedback is all done brother, 😸 |
I can confirm in my own project that I am able to access |
@prograhammer Okay thanks, just needed the confirmation 👍 |
Anything we [at webpack] can help with furthering this along. This is super exciting and we've heard a few downstream requests come our way in regards to its status. Happy to help in any way possible or unblock any blockers if they exist. |
Thanks @TheLarkInn - that's always appreciated! I think the current status is that @prograhammer has fixed all the issues we're aware of and the only thing we're waiting on is @piotr-oles to confirm he's happy to merge. Exciting! @piotr-oles are we ready to go now? |
After turn on the vue option, I got a error: new ForkTsCheckerWebpackPlugin({
vue: true
}) {
test: /\.ts$/,
exclude: /node_modules|vue\/src/,
loader: 'ts-loader',
options: {
transpileOnly: true,
appendTsSuffixTo: [/\.vue$/]
}
},
{
test: /\.vue$/,
loader: 'vue-loader',
options: {
esModule: true,
loaders: {
ts: 'ts-loader',
scss: 'vue-style-loader!css-loader!sass-loader'
}
}
} Starting type checking service...
Using 1 worker with 2048MB memory limit
webpack building...
D:\Code\nei7\node_modules\fork-ts-checker-webpack-plugin\lib\service.js:22
throw error;
^
TypeError: Cannot read property 'flags' of undefined
at getBaseTypes (D:\Code\nei7\node_modules\typescript\lib\typescript.js:28172:37)
at getSuperClass (D:\Code\nei7\node_modules\typescript\lib\typescript.js:37548:21)
at isPropertyDeclaredInAncestorClass (D:\Code\nei7\node_modules\typescript\lib\typescript.js:37537:29)
at checkPropertyNotUsedBeforeDeclaration (D:\Code\nei7\node_modules\typescript\lib\typescript.js:37507:21)
at checkPropertyAccessExpressionOrQualifiedName (D:\Code\nei7\node_modules\typescript\lib\typescript.js:37477:13)
at checkPropertyAccessExpression (D:\Code\nei7\node_modules\typescript\lib\typescript.js:37448:20)
at checkExpressionWorker (D:\Code\nei7\node_modules\typescript\lib\typescript.js:40582:28)
at checkExpression (D:\Code\nei7\node_modules\typescript\lib\typescript.js:40529:42)
at checkExpressionCached (D:\Code\nei7\node_modules\typescript\lib\typescript.js:40406:38)
at checkVariableLikeDeclaration (D:\Code\nei7\node_modules\typescript\lib\typescript.js:42623:43) |
Hi @keenwon
|
Looks good, will be released as 0.3.0 :) Great work! |
@prograhammer this project can reproduce the problem. |
It looks like you found a strange TypeScript bug which is not coming from For example, take this class with the error
and put it in a regular I'll report the issue to TypeScript (as well as look into it myself) and put the link here for anyone to follow. In the mean time, just fix that error in that class. Let us know if anything else breaks, thanks! |
Perhaps a silly question, but why not just swap the foo and bar lines? It seems like a legitimate TypeScript error to me... |
Yeah but it's an error TypeScript should be able to handle without breaking. It handles this error fine outside of experimental decorators (reports it as |
Hmmm weird - definitely report it on the Typescript repo I reckon |
I tried the new release with vue: true and tslint: true, and it works really well. Only issue I found i that linter is running on empty script tag content, even when a <template src="./App.html"></template>
<style scoped src="./App.css"></style>
<script lang="ts" src="./App.ts"></script> This raises tslint errors for missing EOL
|
@Toilal can you upload your working example? in my nuxt project linting is still not working on vue files :/ In my case But if I disable this plugin, everything is working |
Wow ok my error is here... @prograhammer https://github.com/Realytics/fork-ts-checker-webpack-plugin/pull/77/files#r159148429 haha '~' is super common to! (In Microsoft environments more) I'll made a PR for this :D |
That's weird since vue-parser adds an EOL here: https://github.com/prograhammer/vue-parser/blob/master/src/index.ts#L27 Maybe it's complaining about the Vue file itself. Can you add an EOL to it and see what happens:
|
@CKGrafico Of course you know what the best solution is right? Switch to Ubuntu Gnome 3 for development (and dual boot with Windows for gaming/photoshop/etc.). 😆 😆 😆 |
I can't reproduce the EOL issue now ... Maybe after upgrading to tslint 5.9.1 ? (5.9.0 was broken at release, but shortly fixed) |
Hi guys,
This PR adds ability to work with
.vue
files (see #70).For a quick test, I created a
vue
branch (out of thefeature/vue
branch) with the builtlib
folder included. So you can install like this:npm install git://github.com/prograhammer/fork-ts-checker-webpack-plugin.git#vue --save-dev
.vue
files, you need to ensure your script tag's language attribute is setto
ts
ortsx
(also make sure you include the.vue
extension in all your import statements as shown below):tslint.json
looks something like:tsconfig.json
doesn't exclude .vue files:If you are working in VSCode, you'll need extensions Vetur and TSLint Vue (a forked extension which I also currently maintain) and the editor will match the output you get from this
fork-ts-checker-webpack-plugin
.Let me know what more you need or if you have any suggestions on code improvement/cleanup/etc.
Thanks!